Chore(webapp): adds more copy buttons #2529
Conversation
|
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/webapp/app/components/primitives/CopyableText.tsx (2)
40-40: Consider more descriptive padding logic.The
asChildprop addsp-1padding to the button. While this works, the prop name doesn't clearly convey that it affects padding. Consider either:
- Using a more descriptive prop name like
compactorwithInnerPadding- Adding a comment explaining why
asChildtriggers padding- Documenting this behavior in a prop comment
7-17: Add JSDoc documentation for the new prop.Consider adding documentation to explain when and why to use the
asChildprop, especially since it prevents illegal button-in-button markup (as mentioned in the PR objectives).export function CopyableText({ value, copyValue, className, asChild, }: { value: string; copyValue?: string; className?: string; + /** + * When true, prevents button-in-button markup by rendering the tooltip + * as a child element and adds extra padding to the copy button. + * Use this when CopyableText is nested inside interactive elements like links. + */ asChild?: boolean; }) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/components/primitives/CopyableText.tsx(3 hunks)apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx(9 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/components/primitives/CopyableText.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/components/primitives/CopyableText.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/components/primitives/CopyableText.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx
🧬 Code graph analysis (1)
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx (2)
apps/webapp/app/components/primitives/CopyableText.tsx (1)
CopyableText(7-61)apps/webapp/app/utils/pathBuilder.ts (2)
v3RunSpanPath(304-312)v3RunRedirectPath(292-298)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx (6)
427-431: LGTM! Consistent implementation of copy functionality.Good use of the
asChildprop here to prevent button-in-button markup when CopyableText is inside a link.
774-776: LGTM! Region icon rendering with location check.Good defensive check ensuring
region.locationexists before rendering the FlagIcon.
434-435: Consistent tooltip text improvements.The tooltip texts have been consistently updated to "View runs filtered by..." and "View batches filtered by..." which provides clearer user guidance. Good improvement for UX consistency.
439-476: Well-structured Root & Parent run section refactor.The new implementation clearly distinguishes between:
- Combined "Root & Parent run" (when root is also the parent)
- Separate "Root run" and "Parent run" entries
This provides better clarity for users and maintains consistent copy functionality across all run IDs.
597-597: Good distinction between development and deployment versions.The implementation correctly:
- Shows plain copyable text for development environments
- Shows a linked copyable version for deployment environments
This maintains appropriate navigation while adding copy functionality.Also applies to: 610-610
815-816: Approve — run.friendlyId and run.id are appropriate to copyVerified: both identifiers are used widely (DB unique friendlyId, presenters, APIs, UI) and CopyableText is used for similar IDs elsewhere; no change required.
3 participants
You can now more easily copy data from the Details tab on the run page:
asChildto the CopyableText component to prevent illegal button in button markuptooltips.mp4